-
Notifications
You must be signed in to change notification settings - Fork 343
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce remote spawner functionality #5621
Conversation
7fb55c5
to
728c8fe
Compare
@richtja @clebergnu As the concept and changes of the remote spawner is very similar to those of the LXC spawner but simply uses aexpect sessions and API instead of LXC API I wonder what is your overall opinion about such a possibility. Are you for or against supporting remote test spawning? What do you think about the aexpect (potentially in-house) backend and thus dependency for this implementation? I have some linting to fix and a few isolation tests to add but right now I am just curious about your overall thoughts about this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
avocado/plugins/spawners/remote.py
Outdated
""" | ||
|
||
async def wrapper(self, runtime_task): | ||
with RemoteSpawner.reserve_slot(self, runtime_task) as slot: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is identical (besides the "RemoteSpawner" reference that can become a variable) to the version on the lxc spawner. Maybe come up with a version that suits both and share it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I guess we could refactor a shared mechanism for using "available slots" for these types of spawners. Will take a look.
avocado/plugins/spawners/remote.py
Outdated
return status, output | ||
|
||
@contextlib.contextmanager | ||
def reserve_slot(self, runtime_task): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also pretty much identical to the lxc version... It makes an even stronger point towards sharing more of this code IMO.
avocado/plugins/spawners/remote.py
Outdated
import os | ||
import shlex | ||
|
||
from aexpect import remote |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avocado doesn't have a dependency on aexpect. So we have two options (in order of preference):
- Make it an optional plugin
- Protect against import errors (and when/if Spawner: require runtime operation suitability #5649 is merged, signal that in
is_operational()
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, I can do something similar to the LXC_AVAILABLE where I also suggested it gets into the is_operational
method here. I think then I will wait for the above pull request to get merged and will update this pull request accordingly right after.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, the is_operational
method will go a long way and will probably solve this. But, depending on another external library in a "built in" plugin would:
- require either an "import protection" (try/import/except)
- require development mode of the core avocado to have aexpect
IMO, the best solution here is to have this plugin (and any other with extra Python package deps) as an optional_plugin. I tried to come up with an example, and ended up with this working version: https://github.com/clebergnu/avocado/commits/pevogam_remote_spawner_review
Please consider that approach, and feel free to cherry-pick those commits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, I am not sure about your opinion about having an optional LXC plugin then and the comments above about refactoring code from an optional and core plugin (or perhaps two optional plugins) seems harder if at least one of them is optional. Where would you recommend we refactor functionality as in your comment #5621 (comment) and #5621 (comment)? If you prefer of course we might as well not refactor anything from the plugins yet until both of them are fully stabilized over the longer term and simply end this with your effort to migrate the remote spawner to an optional plugin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
9346e8d
to
302ea5e
Compare
Hi @clebergnu, I will make sure our integration checks pass and squash all remaining commits by the end of the day. Until then, the optional plugin usage you suggested causes static checks to fail like:
|
423381e
to
cd7b6f4
Compare
Ok, I pushed a fix on top of 102.0 at first but then noticed that the aexpect dependency is missing from most recent master so decided to simply rebase there. I also reviewed your optional plugin commit and I hope you don't mind that I patched the descriptions (seems to be a copy paste from a different plugin) as follows: diff --git a/python-avocado.spec b/python-avocado.spec
index 080073cc4..13d5caf4d 100644
--- a/python-avocado.spec
+++ b/python-avocado.spec
@@ -384,13 +384,12 @@ a dedicated sever.
%{python3_sitelib}/avocado_framework_plugin_result_upload*
%package -n python3-avocado-plugins-spawner-remote
-Summary: Avocado Plugin to propagate Job results to a remote host
+Summary: Avocado Plugin to spawn tests on a remote host
License: GPLv2+
Requires: python3-avocado == %{version}-%{release}
%description -n python3-avocado-plugins-spawner-remote
-This optional plugin is intended to upload the Avocado Job results to
-a dedicated sever.
+This optional plugin is intended to spawn tests on a remote host.
%files -n python3-avocado-plugins-spawner-remote
%{python3_sitelib}/avocado_spawner_remote*
diff --git a/requirements-dev.txt b/requirements-dev.txt
index 997dc3ad0..60b4bfcc6 100644 Finally, I squashed all commits to a few final ones (no draft commits left) and added a few of your leftover comments from the first review which were temporarily outdated before resolving them. The only thing left from the CI that I can see is a few failing rpm-build variants for CentOS and similar distros. |
No, you understand this exactly right.
What I mean is simply that the "slot approach" is similar to the way it is done for LXC containers and that you will have to define "persistent slots" (your embedded boards that persist and remain the same the testing) in a similar fashion. So it should be fairly easy to adapt for your use case. We do lack some more significant documentation for the time being since these features are too new, experimental, and purely additive to the code base so far but I have used such remote spawners for some distributed testing for almost a year now without having much trouble. |
Okay, I guess I see the big picture better now. I will try. Thanks for your pointers! |
Okay, I got it somewhat working, but boy, this needs documentation :) If someone else wants to try it, first add the slot to your avocado config file:
Then you need a JSON file of the same name as the slot. Its contents are the command line parameters of the
The detail I missed so far: the remote site also needs avocado installed. It makes sense (considering e.g. python native tests), but is sadly a show stopper for me and my limited systems under test. Also, accessing remotes via serial port would be nice to have in my case. That seems to be still missing in aexpect. So, thanks for the help up to here. But I think I am better off writing my tests in |
Oh forgot to say, seeing avocoado finally accessing the board was fun! |
Great investigation and we will make to use the documentation you provided. On my side we use a lot of additoinal automation that not only prepares and provides such configuration but also manages the remote hosts entirely so I could not provide much simpler hints before.
I am not so sure about that though, perhaps the avocado maintainers could provide better advice than me here but I think you would only need a task runner and a few other modules to be copied over. Avocado also considers every executable a potential test so you don't need python native tests for instance.
I am also not sure about this, Avocado VT for instance has serial sessions that are created via aexpect like so def _create_serial_console(self):
"""
Establish a session with the serial console.
Let's consider the first serial port as serial console.
Note: requires a version of netcat that supports -U
"""
if self.serial_session_device is None:
LOG.warning("No serial ports defined!")
return
log_name = "serial-%s-%s.log" % (
self.serial_session_device, self.name)
self.serial_console_log = os.path.join(utils_misc.get_log_file_dir(),
log_name)
file_name = self.get_serial_console_filename(
self.serial_session_device)
self.serial_console = aexpect.ShellSession(
"nc -U %s" % file_name,
auto_close=False,
output_func=utils_misc.log_line,
output_params=(log_name,),
prompt=self.params.get("shell_prompt", "[\#\$]"),
status_test_command=self.params.get("status_test_command",
"echo $?")) @ldoktor Perhaps you could say much more than me especially since you mentioned before lots of users might be interested to apply aexpect in a minimal (often embedded systems) setting. |
Hello @wsakernel I don't know the history, but if you're used to python than As for this runner I can't tell (haven't studied it much) but overall avocado should give you the separation and uniformity as well as various result formats. |
Okay, it may not be a complete Avocado install, but for some use cases, even installing Python3 on the target is already too much. Think of testing bootloaders of an embedded device. But agreed, for tests covering the Linux Kernel, Python3 and a bit of Avocado is doable.
Well, this needs But it is not too bad for me currently. I can do custom |
Hey, ldoktor. Thank you for helping me as well. Much appreciated! The thing is, I am not used to Python. I can read and modify it somewhat, but I am far more fluent in C and shell scripting. To meet my deadlines next week, I will stick to that for now. I am definately interested in using |
Unfortunatelly there is none, just the docstrings. The best you can do is install aexpect, run >>> import aexpect
>>> import time
>>> dir(aexpect)
['Expect', 'ExpectError', 'ExpectProcessTerminatedError', 'ExpectTimeoutError', 'ShellCmdError', 'ShellError', 'ShellProcessTerminatedError', 'ShellSession', 'ShellStatusError', 'ShellTimeoutError', 'Spawn', 'Tail', '__builtins__', '__cached__', '__doc__', '__file__', '__loader__', '__name__', '__package__', '__path__', '__spec__', 'client', 'exceptions', 'kill_tail_threads', 'remote', 'rss_client', 'run_bg', 'run_fg', 'run_tail', 'shared', 'utils']
>>> session = aexpect.ShellSession("bash")
>>> session.cmd("ls /tmp/b")
'1 2\n'
>>> session.cmd("cat /tmp/b/1")
'Hello\n'
>>> session.cmd("cat /tmp/b/2")
'World\n'
>>> dir(session)
['_ShellSession__RE_STATUS', '__class__', '__del__', '__delattr__', '__dict__', '__dir__', '__doc__', '__enter__', '__eq__', '__exit__', '__format__', '__ge__', '__getattribute__', '__getinitargs__', '__getstate__', '__gt__', '__hash__', '__init__', '__init_subclass__', '__le__', '__lt__', '__module__', '__ne__', '__new__', '__reduce__', '__reduce_ex__', '__repr__', '__setattr__', '__setstate__', '__sizeof__', '__str__', '__subclasshook__', '__weakref__', '_add_close_hook', '_add_reader', '_aexpect_helper', '_close_aexpect_helper', '_close_reader_fds', '_get_fd', '_join_thread', '_read_nonblocking', '_start_thread', '_tail', 'a_id', 'auto_close', 'close', 'close_hooks', 'closed', 'cmd', 'cmd_output', 'cmd_output_safe', 'cmd_status', 'cmd_status_output', 'command', 'ctrlpipe_filename', 'echo', 'encoding', 'get_command_output', 'get_command_status', 'get_command_status_output', 'get_id', 'get_output', 'get_pid', 'get_status', 'get_stripped_output', 'inpipe_filename', 'is_alive', 'is_defunct', 'is_responsive', 'kill', 'linesep', 'lock_client_starting_filename', 'lock_server_running_filename', 'log_file', 'log_file_fd', 'match_patterns', 'match_patterns_multiline', 'output_filename', 'output_func', 'output_params', 'output_prefix', 'prompt', 'read_nonblocking', 'read_until_any_line_matches', 'read_until_last_line_matches', 'read_until_last_word_matches', 'read_until_output_matches', 'read_up_to_prompt', 'reader_fds', 'reader_filenames', 'readers', 'remove_command_echo', 'remove_last_nonempty_line', 'send', 'send_ctrl', 'sendcontrol', 'sendline', 'server_log_filename', 'set_linesep', 'set_log_file', 'set_output_func', 'set_output_params', 'set_output_prefix', 'set_prompt', 'set_status_test_command', 'set_termination_func', 'set_termination_params', 'shell_pid_filename', 'status_filename', 'status_test_command', 'tail_thread', 'termination_func', 'termination_params', 'thread_name']
>>> session.sendline("for I in $(seq 10); do echo $I; sleep 1; done")
>>> session.read_nonblocking(0.1, 2)
>>> time.sleep(10)
>>> session.read_nonblocking(0.1, 2)
'2\n3\n4\n5\n6\n7\n8\n9\n10\n[medic@fedora ~ \x1b[1;31m\x1b[0m]$ '
>>> session.sendline("for I in $(seq 10); do echo $I; sleep 1; done")
>>> session.read_nonblocking(1.5, 2)
'1\n2\n3\n4\n'
>>> session.sendline("for I in $(seq 10); do echo $I; sleep 1; done")
>>> session.read_until_output_matches("3", timeout=10)
(0, '1\n2\n3\n')
>>> session.sendline("for I in $(seq 10); do echo $I; sleep 1; done")
>>> session.read_until_output_matches(["5", "7", "2", "foo"], timeout=10)
(2, '1\n2\n')
>>> session.cmd_status("true")
0
>>> session.cmd_status("false")
1
>>> session.is_alive()
True
>>> session.is_responsive()
True
>>> session.sendline("exit")
>>> session.is_alive()
False
>>> session.is_responsive()
False |
@ldoktor: Thank you for this starting point! It is easily understandable, so if I get to it somewhen, I will start here. |
Btw the best benefit of python is the debugging. So let me recommend you Eclipse with pydev, then on the machine where you intend to debug python code install pydevd ( I find this very useful and easier than the usual way where you need to run debug mode from IDE, this way you can run multiple iterations at once and only when it hits the |
This is a great piece of advice in fact, the only downside I have with pydev here is the lack of pure command line frontend. For a more extensive list of alternatives I recommend checking out: |
@pevogam about the packit build failures:
So, the most logic thing to do is to disable the spawner plugin RPM packages on all EL* distros until the situation is normalized. If you could, please add the following: diff --git a/python-avocado.spec b/python-avocado.spec
index 2df3ee6b1..c4437243f 100644
--- a/python-avocado.spec
+++ b/python-avocado.spec
@@ -46,7 +46,9 @@ BuildRequires: python3-docutils
BuildRequires: python3-lxml
BuildRequires: python3-psutil
BuildRequires: python3-setuptools
+%if ! 0%{?rhel}
BuildRequires: python3-aexpect
+%endif
%if ! 0%{?rhel}
%if ! 0%{?fedora} > 35
@@ -132,9 +134,11 @@ popd
pushd optional_plugins/result_upload
%py3_build
popd
+%if ! 0%{?rhel}
pushd optional_plugins/spawner_remote
%py3_build
popd
+%endif
rst2man man/avocado.rst man/avocado.1
%install
@@ -170,9 +174,11 @@ popd
pushd optional_plugins/result_upload
%py3_install
popd
+%if ! 0%{?rhel}
pushd optional_plugins/spawner_remote
%py3_install
popd
+%endif
mkdir -p %{buildroot}%{_mandir}/man1
install -m 0644 man/avocado.1 %{buildroot}%{_mandir}/man1/avocado.1
mkdir -p %{buildroot}%{_pkgdocdir}
@@ -382,6 +388,7 @@ a dedicated sever.
%{python3_sitelib}/avocado_result_upload*
%{python3_sitelib}/avocado_framework_plugin_result_upload*
+%if ! 0%{?rhel}
%package -n python3-avocado-plugins-spawner-remote
Summary: Avocado Plugin to spawn tests on a remote host
License: GPLv2+
@@ -393,6 +400,7 @@ This optional plugin is intended to spawn tests on a remote host.
%files -n python3-avocado-plugins-spawner-remote
%{python3_sitelib}/avocado_spawner_remote*
%{python3_sitelib}/avocado_framework_plugin_spawner_remote*
+%endif
%package -n python3-avocado-examples
Summary: Avocado Test Framework Example Tests I'd also suggest a squash of the commits. IMO, there's no really any benefit to keeping the "original" location of the plugin (it will often require a Other than those, we're ready to merge the plugin IMO. |
Alright, adding it now 👍 |
Thanks @pevogam ! Looks like you need a rebase against the latest master (that should fix the readthedocs.org failure). |
Was wondering whether to write about that first but wanted to see the tests complete - I think merging against the most current master will make this test pass anyway but I always advocate for rebasing since it keeps this the multi-branch git history much cleaner. |
This is minimal functionality excluding potential async enhancements for aexpect. It mainly draws inspiration and mimics slot and other code from the LXC spawner with the exception of extra configurable test timeout that is enforced by aexpect as a dependency and fully specific to this type of spawner. Signed-off-by: Plamen Dimitrov <[email protected]>
The bulk of runner messages sent to the status server via the network are also included in the runner's stdout which ends up hogging the entire aexpect process. Since such stdout is not used anywhere and our debug.log is already collected via the messages straight to the status server, dropping it already helps the test perform on par with equivalent tests from other spawners. Note that it is not worth implementing additional functionality to still allow the stdout stream since even if we run `task-run` from the command line for debugging purposes, it's quite easy to set up a server and lookt at its output instead. This is even more essential when it comes to remote / distributed test runs. Signed-off-by: Plamen Dimitrov <[email protected]>
Even though we only expect to have one spawned task per LXC container or remote host at any given time, the current approach is more future proof with respect to architectural changes. Signed-off-by: Plamen Dimitrov <[email protected]>
Signed-off-by: Cleber Rosa <[email protected]>
There are some packit build failures related to the fact that: 1) centos-stream-8 and centos-stream-9 do not have python3-aexpect, so they're unable to build there 2) only epel-8 has python3-aexpect (not available on epel-9) Until the situation is normalized and aexpect becomes available on those, let's simply disable the remote spawner plugin RPMs for all these distros. Signed-off-by: Plamen Dimitrov <[email protected]>
9b6e7d9
to
58c242e
Compare
Now I seem to get
but this pull request introduced no self-tests I can think of. |
The addition of the new spawner on |
The addition of the new spawner on selftests/check.py has a multiplication effect on that suite. While this seems somewhat internal and fragile, let us simply adapt the check script accordingly. Signed-off-by: Plamen Dimitrov <[email protected]>
Alright everything passed, thanks for the hints at various points! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM @pevogam. Thanks for the patience and all the work here!
This pull request is in similar style and spirit to #4158 but makes use of remote aexpect sessions (to remote hosts or equivalently remote containers behind remote hosts forwarded via specific ports) as slots to schedule test runs on.
TODO: Currently highly experimental and rebased on our LXC spawner branch, to be considered for review and completion once the LXC spawner pull request is merged.